Fix #2131 - Make -ORBListenerInterfaces work properly as documented and tested #2132
Conversation
|
I wouldn't put all gitignore files as part of this PR, when you want to add that, make that a separate PR. Also by using the C++ uniform initialization, std::unique_ptr, std instead of ACE_OS this could be more modern C++. I haven't looked at the issue in detail, this probably needs some detailed review, also the fact that you mention that your approach is maybe not the best will trigger review |
Done in #2133. The changes will disappear from here when that's merged and this is rebased.
Do you have specific callouts? I actually took pains to use the same techniques, tools, and styles used elsewhere in the code that I was changing, because it was not clear to me which things (like
I certainly welcome the review. I came up with the best solution that I knew how to, but I do not know this code nearly as well as do you, and it's possible that you just want to do it a different way, or maybe you'll even do a complete refactor of the affected areas to achieve other goals I'm not aware of. I resolved the Fuzz issues, and I worked through all the Codacy issues that I thought were valid. Two of them are clearly false positives caused by the use of the I'm waiting on platform-specific build results now. |
|
Should "ORBListener" by replaced by "ORBListen"? See |
|
Master requires c++14 or newer |
|
New test should be added to bin/lst file |
I just found |
nickwilliams-zaxiom
force-pushed
the
ORBListenerInterfaces_IPv6_fix
branch
from
a86a104 to
8cceee4
Compare
|
I've rebased to remove all the unrelated One thing I did notice through all of this work is that there are about half a dozen different places that use |
|
Please open a new issue for the getifaddrs/GetAdaptersAddresses proposal, that way it doesn't get lost. |
|
It's been 16 months since this pull request was proposed and 15 months since all code review comments were addressed. Is there anything stopping it from being merged? |
I lack time to do a final review (only did a partial review) and check all builds/tests to make sure nothing breaks by accident. |
All tests pass with this change. The pull request summary page used to reflect that, but it seems that the build configuration for pull requests has been removed or disabled, so the build results are no longer visible. I get the desire to want to make sure nothing breaks by accident, but that's a theoretical problem up against something that is known to be broken: IPv6 is not currently fully supported in TAO. This pull request fixes that known broken feature. I did all the heavy lifting of debugging and researching the problem, experimenting with various fixes, implementing a fix, extensively documenting the problem and my fix, adding new tests for my fix, and making various adjustments in response to your initial review comments. This pull request represents approximately three weeks of my effort. In return for my effort, I would hope you could be a good steward of the open source community by finding one hour of time over a period of 16 months to review the pull request and merge it. |
|
I appreciate all pull requests, issues, feedback but at the end of the month @RemedyIT has to pay its bills. We do a lot of work for free for the ACE/TAO community but that time is getting more and more limited. I find your PR a higher risk one on a first check and I really would need to sit down and go through all the details, setup some builds and check the results (the github actions only compile the core of ACE/TAO, none of the tests and compiled and/or executed, that these are green says not much). That will all together really take more than one hour, you have to wait until I can find some free time or see if you fund the review/test, see https://github.com/DOCGroup/ACE_TAO/wiki/ACE-and-TAO-Commercial-support. All wchar/ascii conversions in your patch are something I find suspicious, wchar to ascii means just remove 50% of the data, converting back from ascii to wchar means adding all 0 bytes. |
WalkthroughThis pull request introduces enhancements to network interface handling across multiple components of the ACE and TAO libraries. The changes primarily focus on adding interface name management capabilities to network address classes, improving multicast address handling, and introducing a new test suite for multicast listener interfaces. The modifications span several files, including Changes
Sequence DiagramsequenceDiagram
participant Client
participant Server
participant NetworkInterface
participant ORB
Client->>Server: Initialize with specific interface
Server->>NetworkInterface: Set interface name
Server->>ORB: Configure listener interfaces
Client->>Server: Shutdown request
Server->>ORB: Perform shutdown
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (16)
TAO/orbsvcs/tests/Miop/McastListenerInterfaces/if_addrs_helper.cpp (5)
1-16: Consider consolidating includes and verifying macro usage.The sequence of includes and macro definitions is correct for each platform, but consider grouping them more cohesively (e.g., Windows- or POSIX-specific) to improve readability. Additionally, verify that
IPV4_MIN_LENGTHandIPV6_MIN_LENGTHreflect realistic minimal string lengths in all cases (for example, IPv6 addresses can include scope IDs which may extend the string).
29-54: Provide RAII-like handling and thorough error reporting.The
WinsockHelperconstructor and destructor correctly manage Winsock initialization/cleanup. However, consider:
- Reporting detailed error messages or returning an error code in addition to printing to
std::cout.- Wrapping the class usage in more robust RAII structures if expanded for more complex logic in the future.
169-205: Ensure robust error handling inaddress_to_string.While the function gracefully handles conversion failures from
WSAAddressToStringW, consider:
- Logging the target family (IPv4/IPv6) during errors for better diagnostics.
- Handling scope IDs for IPv6 addresses, if relevant to your use case.
206-298: Break out detection logic into reusable functions.The main function logically enumerates adapters, determines interface types (loopback vs. Ethernet), and terminates once it finds each combination. This logic is correct but can be split into smaller functions to ease testing and maintenance (e.g., a function to process a single adapter).
301-433: Unify Windows and POSIX code paths if feasible.The POSIX path largely duplicates the Windows logic. To reduce duplication, consider creating a common API for enumerating interfaces, then providing separate small platform-specific implementations. This DRY approach simplifies maintenance and testing for both platforms.
TAO/orbsvcs/tests/Miop/McastListenerInterfaces/Test.idl (1)
1-12: Add documentation or comments around theshutdownoperation.While the IDL is minimal and correct, adding a brief comment explaining the behavior and constraints of
shutdown(e.g., any additional completion notifications) can help future maintainers understand the interface requirements.TAO/orbsvcs/tests/Miop/McastListenerInterfaces/Test_impl.cpp (1)
4-8: Confirm exception handling around_duplicate.The constructor usage of
CORBA::ORB::_duplicateis standard, but consider how exceptions (e.g., iforbis null) will be handled. At minimum, log or assert thatorbis valid, to help diagnose misconfiguration.TAO/orbsvcs/tests/Miop/McastListenerInterfaces/Test_impl.h (1)
1-2: Consider using _H instead of _HPP for header guardsWhile both work, using _H is more common in the ACE/TAO codebase for consistency.
-#ifndef TEST_IMPL_HPP -#define TEST_IMPL_HPP +#ifndef TEST_IMPL_H +#define TEST_IMPL_HTAO/orbsvcs/tests/Miop/McastListenerInterfaces/McastListenerInterfaces.mpc (2)
19-21: Remove empty IDL_Files blocksEmpty IDL_Files blocks can be removed as they serve no purpose.
- IDL_Files { - }Also applies to: 31-33
38-39: Remove empty Header_Files blockEmpty Header_Files block can be removed.
- Header_Files { - }TAO/orbsvcs/tests/Miop/McastListenerInterfaces/README (1)
1-15: Improve README formattingThe content is clear but could benefit from better formatting:
- Add a title section
- Use markdown formatting for better readability
+# McastListenerInterfaces Test + +## Purpose This test verifies the proper behavior of the -ORBListenerInterfaces option on IPv4 and IPv6 with values *=ip_addr, *=interface_name, and CopyPreferredInterfaces. The latter requires also using the -ORBPreferredInterface option. +## Test Procedure It does so by doing the following: -1. Calling the local helper utility if_addrs_helper to determine the system name of the loopback interface and +1. Calls the local helper utility `if_addrs_helper` to determine the system name of the loopback interface and ethernet-like interface(s) with routable IPv4 and IPv6 addresses, as well as the correct IP addresses for those interfaces. -2. Creating a matrix of tests across these interfaces, IP addresses, and the *=ip_addr, *=interface_name, and +2. Creates a matrix of tests across these interfaces, IP addresses, and the `*=ip_addr`, `*=interface_name`, and CopyPreferredInterfaces option values. -3. Executing the matrix of tests and asserting a specific failure (but no other failures) for loopback interfaces and +3. Executes the matrix of tests and asserts a specific failure (but no other failures) for loopback interfaces and no failures for non-loopback interfaces. Specifically, for loopback interfaces, the server process should never receive the instruction to shut down from the client, and so the test should time out waiting on the server process and elect to kill it.TAO/orbsvcs/tests/Miop/McastListenerInterfaces/client.cpp (1)
6-6: Avoid global variablesConsider encapsulating the IOR in a class or passing it as a parameter.
TAO/orbsvcs/tests/Miop/McastListenerInterfaces/server.cpp (1)
39-129: Consider adding error handling for memory allocation.The server implementation looks good overall, but there's a potential memory leak risk if
ACE_NEW_RETURNfails afterorb->resolve_initial_references()succeeds.Consider using RAII with
unique_ptr:- Server_impl* server_obj = 0; - ACE_NEW_RETURN (server_obj, - Server_impl(orb.in()), - 3); - PortableServer::ServantBase_var owner (server_obj); + std::unique_ptr<Server_impl> server_obj(new Server_impl(orb.in())); + PortableServer::ServantBase_var owner(server_obj.release());TAO/orbsvcs/tests/Miop/McastListenerInterfaces/run_test.pl (1)
139-288: Consider adding test timeout configuration.The test execution logic is solid, but consider making the timeout values configurable through environment variables for debugging purposes.
Add timeout configuration:
+my $DEFAULT_TIMEOUT = 60; # seconds +my $test_timeout = $ENV{TEST_TIMEOUT} || $DEFAULT_TIMEOUT;ACE/ace/INET_Addr.h (1)
441-441: Consider initializing if_name_ in constructor.While nullptr initialization is fine, consider moving it to the constructor for consistency with other member initializations.
ACE/ace/SOCK_Dgram.cpp (1)
Line range hint
764-897: Consider refactoring IPv6 address lookup logic.The IPv6 address lookup logic is complex and could benefit from being split into a separate method for better maintainability.
Consider extracting the IPv6 address lookup into a separate method:
private: int find_ipv6_interface_index(const char* net_if_char, const in6_addr& net_if_in6_addr) { // Move the IPv6 lookup logic here }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.gitignore(1 hunks)ACE/ace/INET_Addr.cpp(2 hunks)ACE/ace/INET_Addr.h(3 hunks)ACE/ace/SOCK_Dgram.cpp(6 hunks)ACE/ace/Sock_Connect.cpp(2 hunks)TAO/bin/tao_other_tests.lst(1 hunks)TAO/orbsvcs/tests/Miop/McastListenerInterfaces/.gitignore(1 hunks)TAO/orbsvcs/tests/Miop/McastListenerInterfaces/McastListenerInterfaces.mpc(1 hunks)TAO/orbsvcs/tests/Miop/McastListenerInterfaces/README(1 hunks)TAO/orbsvcs/tests/Miop/McastListenerInterfaces/Test.idl(1 hunks)TAO/orbsvcs/tests/Miop/McastListenerInterfaces/Test_impl.cpp(1 hunks)TAO/orbsvcs/tests/Miop/McastListenerInterfaces/Test_impl.h(1 hunks)TAO/orbsvcs/tests/Miop/McastListenerInterfaces/client.cpp(1 hunks)TAO/orbsvcs/tests/Miop/McastListenerInterfaces/if_addrs_helper.cpp(1 hunks)TAO/orbsvcs/tests/Miop/McastListenerInterfaces/run_test.pl(1 hunks)TAO/orbsvcs/tests/Miop/McastListenerInterfaces/server.cpp(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🔇 Additional comments (14)
TAO/orbsvcs/tests/Miop/McastListenerInterfaces/if_addrs_helper.cpp (1)
56-109: Validate buffer reallocation loop and handle partial failures.The buffer reallocation loop for
GetAdaptersAddressesis straightforward; however:
- Ensure partial allocations don't mask other errors (e.g., repeated
ERROR_BUFFER_OVERFLOWcould point to abnormal behavior).- Consider logging the iteration count to ease debugging under extreme conditions.
TAO/orbsvcs/tests/Miop/McastListenerInterfaces/Test_impl.cpp (1)
9-12: Consider atry-catchblock for the shutdown.The
shutdown(false)call might throw exceptions in certain ORB configurations or states. Depending on the system requirements, ensure you have a plan for gracefully handling such exceptions (even if it's logging).TAO/orbsvcs/tests/Miop/McastListenerInterfaces/server.cpp (1)
10-37: LGTM! Well-structured argument parsing.The command-line argument parsing is well-implemented with clear error handling and usage messages.
TAO/orbsvcs/tests/Miop/McastListenerInterfaces/run_test.pl (2)
1-97: LGTM! Robust test configuration setup.The test configuration setup is thorough, with proper error handling for the
if_addrs_helpercommand and comprehensive interface type detection.
99-137: LGTM! Well-documented helper functions.The helper functions are well-documented and handle their specific tasks effectively.
ACE/ace/INET_Addr.h (2)
22-24: LGTM! Appropriate header includes.The addition of memory and string headers is appropriate for the new functionality.
289-304: LGTM! Well-documented interface name methods.The new methods are well-documented with clear explanations of their informational purpose.
ACE/ace/SOCK_Dgram.cpp (1)
Line range hint
652-723: LGTM! Improved interface name handling.The changes properly handle both interface names and IP addresses, with good fallback behavior for older Linux kernels.
ACE/ace/INET_Addr.cpp (3)
316-319: LGTM! Good use of modern C++.The copy constructor properly handles the interface name copying using std::shared_ptr, with appropriate null checking.
1009-1013: LGTM! Clean getter implementation.The method correctly returns a shared_ptr to const string, maintaining const correctness and memory safety.
1015-1026: LGTM! Well-implemented setter with proper null handling.The method correctly handles both null and non-null cases, using modern C++ features like make_shared.
ACE/ace/Sock_Connect.cpp (1)
610-610: LGTM! Consistent interface name handling for both IPv4 and IPv6.The implementation properly sets interface names for both address families using the new functionality.
Also applies to: 623-625
TAO/bin/tao_other_tests.lst (1)
242-242: LGTM! Test entry properly configured.The test entry has appropriate configuration flags consistent with other MIOP tests in the list.
TAO/orbsvcs/tests/Miop/McastListenerInterfaces/.gitignore (1)
1-8: LGTM! Comprehensive gitignore entries.The file correctly lists all expected generated files and build artifacts for the test directory.
| bool | ||
| is_link_local (const struct in6_addr * a) | ||
| { | ||
| // fe80::/64 | ||
| return (a->s6_bytes[0] == 0xfe && | ||
| a->s6_bytes[1] == 0x80 && | ||
| a->s6_words[1] == 0 && | ||
| a->s6_words[2] == 0 && | ||
| a->s6_words[3] == 0); | ||
| } | ||
|
|
||
| bool | ||
| is_site_local (const struct in6_addr * a) | ||
| { | ||
| // fec0::/16 | ||
| return (a->s6_bytes[0] == 0xfe && | ||
| a->s6_bytes[1] == 0xc0); | ||
| } | ||
|
|
||
| bool | ||
| is_v4_mapped (const struct in6_addr * a) | ||
| { | ||
| // ::ffff:0:0/96 | ||
| return (a->s6_words[0] == 0 && | ||
| a->s6_words[1] == 0 && | ||
| a->s6_words[2] == 0 && | ||
| a->s6_words[3] == 0 && | ||
| a->s6_words[4] == 0 && | ||
| a->s6_words[5] == 0xffff); | ||
| } | ||
|
|
||
| bool | ||
| is_v4_translated (const struct in6_addr * a) | ||
| { | ||
| // ::ffff:0:0:0/96 | ||
| return (a->s6_words[0] == 0 && | ||
| a->s6_words[1] == 0 && | ||
| a->s6_words[2] == 0 && | ||
| a->s6_words[3] == 0 && | ||
| a->s6_words[4] == 0xffff && | ||
| a->s6_words[5] == 0); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use standardized IPv6 macros for classification.
All helper functions (is_link_local, is_site_local, is_v4_mapped, is_v4_translated) replicate functionality offered by macros such as IN6_IS_ADDR_LINKLOCAL, etc. While your custom checks may be valid, standardized macros reduce the risk of bitwise errors and increase maintainability.
| bool | ||
| is_loopback (const struct in6_addr * a) | ||
| { | ||
| // ::1 | ||
| return (a->s6_words[0] == 0 && | ||
| a->s6_words[1] == 0 && | ||
| a->s6_words[2] == 0 && | ||
| a->s6_words[3] == 0 && | ||
| a->s6_words[4] == 0 && | ||
| a->s6_words[5] == 0 && | ||
| a->s6_words[6] == 0 && | ||
| a->s6_bytes[14] == 0 && | ||
| a->s6_bytes[15] == 0x01); | ||
| } |
There was a problem hiding this comment.
Confirm endianness assumptions for IPv6 loopback checks.
These byte and word checks in is_loopback rely on raw struct fields. Typically, IPv6 addresses are stored in network byte order. Validate that these checks succeed on little-endian and big-endian systems. If strict portability is required, consider using standard macros like IN6_IS_ADDR_LOOPBACK.
Proposed fix example:
-bool is_loopback (const struct in6_addr * a)
-{
- return (a->s6_words[0] == 0 && ... && a->s6_bytes[15] == 0x01);
-}
+bool is_loopback (const struct in6_addr * a)
+{
+ return IN6_IS_ADDR_LOOPBACK(a);
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bool | |
| is_loopback (const struct in6_addr * a) | |
| { | |
| // ::1 | |
| return (a->s6_words[0] == 0 && | |
| a->s6_words[1] == 0 && | |
| a->s6_words[2] == 0 && | |
| a->s6_words[3] == 0 && | |
| a->s6_words[4] == 0 && | |
| a->s6_words[5] == 0 && | |
| a->s6_words[6] == 0 && | |
| a->s6_bytes[14] == 0 && | |
| a->s6_bytes[15] == 0x01); | |
| } | |
| bool | |
| is_loopback (const struct in6_addr * a) | |
| { | |
| return IN6_IS_ADDR_LOOPBACK(a); | |
| } |
| class Server_impl : public virtual POA_Test::Server | ||
| { | ||
| public: | ||
| Server_impl (CORBA::ORB_ptr orb); | ||
|
|
||
| virtual void shutdown (); | ||
|
|
||
| private: | ||
| CORBA::ORB_var orb_; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add virtual destructor and class documentation
The class is missing:
- A virtual destructor which is necessary for proper cleanup when deleting through base class pointer
- Documentation explaining the class purpose and method behavior
+/**
+ * @class Server_impl
+ * @brief Implementation of the Test::Server interface
+ */
class Server_impl : public virtual POA_Test::Server
{
public:
Server_impl (CORBA::ORB_ptr orb);
+ virtual ~Server_impl() = default;
virtual void shutdown ();
private:
CORBA::ORB_var orb_;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class Server_impl : public virtual POA_Test::Server | |
| { | |
| public: | |
| Server_impl (CORBA::ORB_ptr orb); | |
| virtual void shutdown (); | |
| private: | |
| CORBA::ORB_var orb_; | |
| }; | |
| /** | |
| * @class Server_impl | |
| * @brief Implementation of the Test::Server interface | |
| */ | |
| class Server_impl : public virtual POA_Test::Server | |
| { | |
| public: | |
| Server_impl (CORBA::ORB_ptr orb); | |
| virtual ~Server_impl() = default; | |
| virtual void shutdown (); | |
| private: | |
| CORBA::ORB_var orb_; | |
| }; |
| Test::Server_var server = | ||
| Test::Server::_unchecked_narrow (tmp.in ()); |
There was a problem hiding this comment.
Use _narrow instead of _unchecked_narrow
_unchecked_narrow skips type checking which could lead to runtime errors. Use _narrow for type-safe conversion.
- Test::Server::_unchecked_narrow (tmp.in ());
+ Test::Server::_narrow (tmp.in ());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Test::Server_var server = | |
| Test::Server::_unchecked_narrow (tmp.in ()); | |
| Test::Server_var server = | |
| Test::Server::_narrow (tmp.in ()); |
| orb->destroy (); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add proper cleanup in error paths
The ORB should be destroyed even when errors occur. Consider using RAII or ensure cleanup in all paths.
try
{
CORBA::ORB_var orb = CORBA::ORB_init (argc, argv);
if (parse_args (argc, argv) != 0)
- ACE_ERROR_RETURN ((LM_ERROR, "Wrong arguments\n"), 2);
+ {
+ orb->destroy();
+ ACE_ERROR_RETURN ((LM_ERROR, "Wrong arguments\n"), 2);
+ }Committable suggestion skipped: line range outside the PR's diff.
| if (parse_args (argc, argv) != 0) | ||
| ACE_ERROR_RETURN ((LM_ERROR, "Wrong arguments\n"), 2); |
There was a problem hiding this comment.
Add nullptr check for ior_server
Need to verify ior_server is not null before using it.
if (parse_args (argc, argv) != 0)
ACE_ERROR_RETURN ((LM_ERROR, "Wrong arguments\n"), 2);
+
+if (ior_server == nullptr)
+ ACE_ERROR_RETURN ((LM_ERROR, "No IOR specified\n"), 2);Committable suggestion skipped: line range outside the PR's diff.
See the detailed explanation for this change in #2131.
A quick note about this change: it includes a new test,
$TAO_ROOT/orbsvcs/tests/Miop/McastListenerInterfaces. In order for this test to compile and run,$TAO_ROOT/TAO_ACE.mwchas to be edited to comment out or remove the lineorbsvcs/testsin theexcludeblock. It seems that allorbsvcstests are currently disabled inmaster. I did not include this change in my pull request, because it seems those tests were disabled for a reason, which I won't second-guess. I know, for example, that most of the tests in$TAO_ROOT/orbsvcs/tests/Miop/fail on a clean master.Summary by CodeRabbit
Based on the comprehensive changes, here are the release notes:
New Features
Test Improvements
Bug Fixes
Chores
.gitignoreto exclude macOS-specific filesThe changes primarily focus on network interface management and testing infrastructure, with significant improvements to the ACE library's address handling capabilities.